Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Push AVC validator to mod repos #113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Push AVC validator to mod repos #113

wants to merge 1 commit into from

Conversation

HebaruSan
Copy link
Member

Motivation

@DasSkelett has written a cool GitHub Action to validate version files:

Currently deployment of this is opt-in, as mod authors find out about it. This may limit adoption.

Changes

Now a new netkan avc-validator-pusher command is created that:

  • Looks for netkans that are linked to GitHub repositories and have vrefs
  • Forks and clones them
  • Adds the file needed for the validator
  • Commits it to a branch
  • Submits a pull request for these changes with a brief explanation of what it's all about

This way adoption of the validator can be as easy as clicking Merge. Hopefully this will get a large number of mod authors to sign on. As a bonus, it's a way of sending 200 PRs to LGG with one command.

Note that I want the PR body to link to this PR, so I'll be updating the code after it's created.

Name partially inspired by this song.

@HebaruSan HebaruSan added the Enhancement New feature or request label Dec 19, 2019
@HebaruSan
Copy link
Member Author

Don't merge this till @DasSkelett is back from holiday break in January.

@techman83
Copy link
Member

Codewise it looks ok to me (I haven't had a thorough test/look over it yet), but I have concerns about bulk spraying PRs. I don't know that it's a good way to get buy in for its usage and feels a bit opinionated on our part.

I am however in favour of making this as easy as possible for mod authors to add. It would be real nice to be able to trigger this from the frontend, say via a user logged into GitHub. But that's a much harder exercise.

@HebaruSan
Copy link
Member Author

I don't know that it's a good way to get buy in for its usage and feels a bit opinionated on our part.

Do you think the core concept is salvageable? We could make the PR text more conciliatory, we could make the PR submission manual instead of automated (auto-generating the forks, branches, and commits would still be a huge help), we could limit to one PR per author per week, etc. I would really like to see broad adoption of this tool.

@techman83
Copy link
Member

Absolutely, I don't think the work is a waste at all. We could make it a scheduled task and set a flag on the mod in DynamnoDB. Somehow make it opt in, give us an opportunity to automate things like improvements to the process + raise new PRs.

My only critique was that we want to be careful about our approach here, because It's definitely a GoodThing™. But we want buy in from the community, something that makes their lives easier, rather than CKAN having strong opinions about their workflow.

@HebaruSan
Copy link
Member Author

Rebased onto common argument framework from #122, I think it's right but worth a close look.

@techman83
Copy link
Member

The changes look to fit in with #122. I don't know that we've addressed the non-technical aspects of this PR or how we intend to use this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Enhancement New feature or request In Progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@HebaruSan @techman83